Coverity found this: |set| is deleted in one if-statement, but in the end returned. http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#5985 5985/* static */ nsMsgKeySetU* nsMsgKeySetU::Create() 5986{ 5987 nsMsgKeySetU* set = new nsMsgKeySetU; 1. Condition set, taking true branch 5988 if (set) 5989 { 5990 set->loKeySet = nsMsgKeySet::Create(); 5991 set->hiKeySet = nsMsgKeySet::Create(); 5992 } 2. Condition set, taking true branch 3. Condition set->loKeySet, taking false branch 5993 if (!(set && set->loKeySet && set->hiKeySet)) 4. freed_arg: operator delete frees set. [Note: The source code implementation of the function has been overridden by a builtin model.] 5994 delete set; CID 1137669 (#1 of 1): Use after free (USE_AFTER_FREE)5. use_after_free: Using freed pointer set. 5995 return set; 5996} 5997 Observation: I think we should return nullptr when |delete set| on line 5994 is invoked.

Yes, that is also done at http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgKeySet.cpp#197 .

proposed patch.

Comment on attachment 8762568 [details] [diff] [review] Bug 127960 - set |set| to nullptr after |delete set| before |return set| Review of attachment 8762568 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/base/util/nsMsgDBFolder.cpp @@ +5989,5 @@ > { > set->loKeySet = nsMsgKeySet::Create(); > set->hiKeySet = nsMsgKeySet::Create(); > } > if (!(set && set->loKeySet && set->hiKeySet)) I wonder why we test !set again, when we already tested above that is is not null. Would you agree this test could be put inside the if() above and drop !set from it?

(In reply to :aceman from comment #3) > Comment on attachment 8762568 [details] [diff] [review] > Bug 127960 - set |set| to nullptr after |delete set| before |return set| > > Review of attachment 8762568 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mailnews/base/util/nsMsgDBFolder.cpp > @@ +5989,5 @@ > > { > > set->loKeySet = nsMsgKeySet::Create(); > > set->hiKeySet = nsMsgKeySet::Create(); > > } > > if (!(set && set->loKeySet && set->hiKeySet)) > > I wonder why we test !set again, when we already tested above that is is not > null. Would you agree this test could be put inside the if() above and drop > !set from it? You mean like the new patch. I put Joshua as reviewer. If this is not appropriate, please change this. TIA

Comment on attachment 8762603 [details] [diff] [review] Bug 127960 - set |set| to nullptr after |delete set| before |return set| Review of attachment 8762603 [details] [diff] [review]: ----------------------------------------------------------------- Yes, thanks. I think we do not need to bother jcranmer with this one.

https://hg.mozilla.org/comm-central/rev/7cb60a63a47e7c77fce8cc07d2d142fb3dd06f21 I didn't notice the checkin comment contained incorrect bug number. 127960 instead of 1279560.

(In reply to :aceman from comment #6) > https://hg.mozilla.org/comm-central/rev/ > 7cb60a63a47e7c77fce8cc07d2d142fb3dd06f21 > > I didn't notice the checkin comment contained incorrect bug number. 127960 > instead of 1279560. Aha, thank you for noticing this. I think I need a more or less automated way to copy coverity issue to bugzilla and then create a patch based on correct bugzilla # in checkin comment, etc.